Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[engine] rm python graphmaker; create dot formatted display #4295

Merged

Conversation

baroquebobcat
Copy link
Contributor

Problem

Printing rule graphs has not been converted. Also, generating subgraphs.

Solution

(describe the modifications you've made.)

Result

(describe how your changes affect the end-user behavior of the system. this section is
optional, and should generally be summarized in the title of the pull request.
)

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nick.

product_type: TypeConstraint
) -> RuleGraph {
let graph_maker = GraphMaker::new(&raw.scheduler.tasks,
RootSubjectTypes { subject_types: vec![subject_type.clone()] });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct doesn't appear to be carrying its weight. Could either make it a tuple struct so that you can construct it more easily (RootSubjectTypes(vec![subject_type.clone())) or just remove it, probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I added it thinking it might accrete more behavior, but then it didn't. xxing

let file = try!(OpenOptions::new().append(true).open(path));
let mut f = BufWriter::new(file);

try!(write!(&mut f, "{}\n", format!("{}", graph)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've mostly switched to ? rather than try!... will make a pass over the old code at some point, but might as well use ? in new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating to it.

}

fn write_to_file(path: &Path, graph: &RuleGraph) -> io::Result<()> {
let file = try!(OpenOptions::new().append(true).open(path));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append doesn't seem like a good default... I don't want a graph appended to an existing file, since dot isn't appendable afaik?

Copy link
Contributor Author

@baroquebobcat baroquebobcat Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. This was cargo culted from one of the other file creating fns. -- trace in graph.rs

unfulfillable_rules: full_unfulfillable_rules,
}
}
let beginning_root = beginning_root_opt.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A cleaner way to work with an Option while still returning early might be:

let beginning_root = 
  if let Some(beginning_root) = self.gen_root_entry(subject_type, product_type) {
    beginning_root
  } else {
    return ...
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, my python / java is showing a bit there.

full_dependency_edges = constructed_graph.rule_dependency_edges.clone();
full_unfulfillable_rules = constructed_graph.unfulfillable_rules.clone();

self.add_unreachable_rule_diagnostics(&full_dependency_edges, &mut full_unfulfillable_rules);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly seems like you should be able to pass these in without cloning them... is the issue just that constructed_graph isn't mutable? let mut constructed_graph = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I started to see that when I extracted the method. Thanks for pointing it out.

unfulfillable_rules: full_unfulfillable_rules
};

self._remove_unfulfillable_rules_and_dependents(unfinished_graph)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reminder that this pattern of methods that take a value, mutate it, and then return the same value... feels weird to me. I would expect a method that mutates something and then gives it back to take a &mut to the thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead of taking ownership, modifying and passing back ownership, just doing an &mut is more idiomatic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe so. I'm not sure whether LLVM or rustc will sneakily turn the call into a reference for you, but afaik: moving values in is ... literally moving (ie memcpying) them. The graph is not a large struct (it only has Vecs in it, and they are always 3 words long?) but I think the idea is that when you can pass by reference, you should.

@@ -602,6 +690,54 @@ impl RuleGraph {
}
}

impl fmt::Display for RuleGraph {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an abuse of Display. I wouldn't accidentally want to drop this in a log message, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I agree. I was planning on taking this out, but with the support rotation stuff---I wanted to start the CI runs before fixing it.

@@ -71,7 +71,11 @@ impl Tasks {
}

pub fn all_product_types(&self) -> Vec<TypeConstraint> {
self.all_rules().iter().map(|t| t.product).collect()
let mut product_types: Vec<_> = self.all_rules().iter().map(|t| t.product).collect();
// NB sorted by id so that dedup will consolidate runs of duplicates.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to just collect into a hashset instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should work.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

try!(write!(&mut f, "{}\n", format!("{}", graph)));
let file = File::create(path)?;
let mut f = io::BufWriter::new(file);
graph.visualize(&mut f)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than:

graph.visualize(&mut f)?;
Ok(())

... you can just return the result of visualize:

graph.visualize(&mut f)

@baroquebobcat baroquebobcat merged commit 45507e2 into pantsbuild:master Mar 7, 2017
lenucksi pushed a commit to lenucksi/pants that referenced this pull request Apr 25, 2017
…ld#4295)

### Problem

Printing rule graphs has not been converted to Rust. This can make it hard to visualize what the rule sets look like. Also, the Python rule graph construction is effectively dead code already.

### Solution

Port rule graph visualization and visualization based testing to the rust side of the engine implementation.

### Result

Now we're setup to use the RuleGraph from the Rust side for functions as described in pantsbuild#4303 and pantsbuild#4304.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants